Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(appsync): add dependency between apikey and schema #9737

Merged
merged 13 commits into from
Aug 19, 2020
Merged

fix(appsync): add dependency between apikey and schema #9737

merged 13 commits into from
Aug 19, 2020

Conversation

BryanPan342
Copy link
Contributor

@BryanPan342 BryanPan342 commented Aug 15, 2020 •
edited
Loading

Added dependency between the CfnApiKey and CfnSchema. The dependency here is to prevent a ConcurrencyModificationError as seen in #8168. We allow this dependency to exist because from referencing the docs there shouldn't be any issue between creating an api key before or after schema creation.

Also make ApiKeyConfig correctly configure the ApiKey when used in additionalAuthorizationModes.

Fixes #9736
Fixes #8168

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@BryanPan342 BryanPan342 self-assigned this Aug 15, 2020
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Aug 15, 2020
@BryanPan342 BryanPan342 added @aws-cdk/aws-appsync Related to AWS AppSync and removed contribution/core This is a PR that came from AWS. labels Aug 15, 2020
@BryanPan342
Copy link
Contributor Author

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Aug 15, 2020

Command refresh: success

@MrArnoldPalmer MrArnoldPalmer added the pr/do-not-merge This PR should not be merged at this time. label Aug 19, 2020
MrArnoldPalmer
MrArnoldPalmer previously approved these changes Aug 19, 2020
View reviewed changes
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added do not merge in case you're still adding commits here.

shivlaks
shivlaks previously requested changes Aug 19, 2020
View reviewed changes
Copy link
Contributor

@shivlaks shivlaks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also update the commit body to describe the changes in behaviour we are making and the rationale. i.e. why is the dependency needed, approach, etc

paraphrasing a summary of issues it fixes can be helpful here too since there's some conversation and considerations discussed in them.

packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts Show resolved Hide resolved
packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed stale reviews from MrArnoldPalmer and shivlaks August 19, 2020 17:35

Pull request has been modified.

shivlaks
shivlaks previously approved these changes Aug 19, 2020
View reviewed changes
@BryanPan342 BryanPan342 removed the pr/do-not-merge This PR should not be merged at this time. label Aug 19, 2020
@mergify mergify bot dismissed shivlaks’s stale review August 19, 2020 21:41

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Aug 19, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify
Copy link
Contributor

mergify bot commented Aug 19, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 4448794 into aws:master Aug 19, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 449a4ab
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@BryanPan342 BryanPan342 deleted the apikey branch September 8, 2020 18:33
@cshivashankar cshivashankar mentioned this pull request Nov 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shivlaks shivlaks shivlaks approved these changes

@MrArnoldPalmer MrArnoldPalmer MrArnoldPalmer left review comments

Assignees

@BryanPan342 BryanPan342

Labels
@aws-cdk/aws-appsync Related to AWS AppSync
Projects
None yet
Milestone
[DevPreview] AppSync
4 participants
@BryanPan342 @aws-cdk-automation @MrArnoldPalmer @shivlaks